Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve how commands are internally executed #1247

Merged
merged 7 commits into from
Oct 30, 2023
Merged

Improve how commands are internally executed #1247

merged 7 commits into from
Oct 30, 2023

Conversation

uweseimet
Copy link
Contributor

@uweseimet uweseimet commented Oct 15, 2023

  • Use const CommandContext for execution
  • Update error handling
  • Fix SonarQube issues
  • Remove duplicate code
  • Use mutex instead of atomic_bool

* Use const CommandContext for execution
* Update error handling
* Fix SonarQube issues
* Remove duplicate code
* Use mutex instead of atomic_bool
* Add hasher
* Add param_map
@uweseimet uweseimet marked this pull request as ready for review October 15, 2023 08:13
@uweseimet uweseimet linked an issue Oct 15, 2023 that may be closed by this pull request
3 tasks
@uweseimet
Copy link
Contributor Author

@akuker Less changes this time ;-).

I have not managed to add @dialtr to the list of reviewers. Any idea why? I thought anybody could be added.

if (const string error = service.Init([this] (CommandContext& context) {
context.SetDefaultFolder(piscsi_image.GetDefaultFolder());
return ExecuteCommand(context);
}, port); !error.empty()) {
cerr << "Error: " << error << endl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elsewhere I see reference to a logging function (spdlog::info, etc). Is this write to cerr leftover from debugging and should it be converted to a proper log message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, everything that is logged to a logfile (usually by the PiSCSI core) is logged with the spdlog library. Messages the result from the user launching command line tools (e.g. scsictl, piscsi) are usually written to cout or cerr, in order to provide direct feedback to the user (e.g. because a command line parameter was wrong). There is no need for those to be logged with timestamps etc.

In the old C++ code you may still find macros use for logging, but they should gradually be replaced by direct usage of spdlog. The macros have issues, e.g. there might be a buffer overflow. Current C++ tries to reduce the use of macros anyway because of several reasons.

Which IDE or editor are you using, by the way? And which kind of Pi? My main development environment is Eclipse on Linux, a Pi 4, and mainly Atari computers for testing that requires SCSI functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Yes, down with macros!

Typically, I use vim as an editor, but have also been using CLion lately. I note that you have a clang-format configuration file defining the formatting style, and CLion plays nicely with that, because it can enforce formatting rules. Thus, I am likely to use CLion more for this project.

In "production", my PiSCSI is connected to a Raspberry Pi 3 Model A+, but I also have a 4 lying around. I am using this as a drive emulator for my Akai S3000XL and Akai S6000 samplers, and will also be able to test it on a Kurzweil K2000R sampler at some point.

While the Tindie store is currently out of stock, I am just going to invest in another PiSCSI when they are available so that I can maintain a development system separate from the one that I use for music stuff.

I've forked the repo and have been poking around. Are there build instructions anywhere for Debian amd64? Or is the procedure to run that script?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the formatting style file was added by @akuker. I have never used it, and I do not know how closely it reflects the current style.

If you want to compile anywhere, e.g. on a regular PC, there are notes how to do this at the bottom of https://github.com/PiSCSI/piscsi/wiki/Setup-Instructions.
I guess running easysetup might work (@rdmark probably knows more), but when I set up a new compile environment I usually just check out the sources, install the libraries/headers that may be required for compiling (there is a list of them in the easysetup script), and then just run make.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the formatting style file was added by @akuker. I have never used it, and I do not know how closely it reflects the current style.

@uweseimet How about we review the coding guidelines and agree upon a shared set of stylistic rules? With a new developer onboarding, I think it would be productive to have this to avoid f.e. unexpected line changes when someone's IDE automatically changes indentation or line breaks, etc.

I think this is the file in question: https://github.com/PiSCSI/piscsi/blob/main/cpp/.clang-format

Let me create a new ticket for this.

Copy link
Contributor Author

@uweseimet uweseimet Oct 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdmark I agree. But I would like to merge all pending PRs and the tickets that are already ready for PRs but already may need merging (there are several) first. Otherwise formatting changes will make the existing changed code unusable. Does this make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uweseimet Yes I agree with that priority. I will create the ticket so that we can take action on this a little bit later.

Comment on lines +88 to +89
// TODO Check why there are rare cases where bus is NULL on a remote interface shutdown
// even though it is never set to NULL anywhere
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a bug report for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but I don't think it is needed. The null check is a working solution, but not one I am satisfied with. All in all this is just something that should be improved, but nothing that affects the user. This is why I think a TODO is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will merge after resolving the merge conflicts.

@uweseimet uweseimet merged commit 8bd06ea into develop Oct 30, 2023
12 of 13 checks passed
@uweseimet uweseimet deleted the issue_1214 branch October 30, 2023 12:57
@sonarcloud
Copy link

sonarcloud bot commented Oct 30, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

20.3% 20.3% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve how Piscsi and PiscsiExecutor execute commands
3 participants